fix(sync): converge channel sidebar state across devices on the same identity - #6525

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Requesting changes at exact head 0d797b550e405eea9557e053f884533ad3dd891a. The relay ordering and the per-entry max-merge are coherent, but the lifecycle still has reproducible edit-loss paths.

[P1] The durable outboxes are not safe across desktop windows

Sections, stars, and mutes share one localStorage outbox key per identity + relay, but each window owns only an in-memory generation. A write in one window therefore replaces another window's pending payload, and a completion in either window removes the shared key without proving that it still owns the persisted value.

For sections, window A can start publishing edit A, window B can replace the outbox with newer edit B, and A's ACK then clears B's outbox. If B closes before its debounce fires, the next bootstrap applies relay head A before discovering there is no outbox, so B is permanently lost. See channelSectionsStorage.ts:211-245, channelSectionsSync.ts:262-281,284-313,522, and useChannelSections.ts:138-156.

Stars and mutes can lose independent clicks even earlier: two windows starting from the same store can click different channels before receiving each other's asynchronous storage event. Their whole-store main/outbox writes race, teardown cancels both timers, and remount resumes only the last blob. The storage handler merges only into React state; it does not durably merge the main store or outbox. See channelStarsStorage.ts:206-242, useChannelStars.ts:57-72,95-113, and channelStarsSync.ts:256-260,383-393 (mutes mirror these paths).

Please give persisted attempts cross-window ownership, not only manager-local generations. A per-operation outbox whose owner deletes only its own record, or an actual cross-window serialization mechanism, would close both overwrite and stale-clear races. Add multi-window tests that interleave write, ACK, storage delivery, teardown, and remount.

[P1] Sort preferences still drop pending edits on ordinary lifecycle and failure paths

publishSortPrefs keeps intent only in memory (channelSortSync.ts:113-121). destroy() deliberately cancels and discards it (:252-262), while a failed publish only logs and never retries (:167-210). A live remote also cancels the debounce while leaving pendingStore stranded (useChannelSortPreference.ts:82-103).

Reproduction: change a sort mode and quit/switch communities within two seconds, or let one publish time out and remount. Bootstrap then whole-blob-replaces the local cache with the relay head (useChannelSortPreference.ts:108-122), visibly reverting the user's choice. Please carry the durable outbox, generation ownership, serialized retry, and pending-aware remote application used by sections over to sort, with lifecycle tests.

[P2] Future relay heads can wedge stars, mutes, and sort publishing

Sections clamps created_at inside the relay's future-drift window (channelSectionsSync.ts:463-472), but stars (channelStarsSync.ts:298-301), mutes (matching code), and sort (channelSortSync.ts:183-186) stamp lastRemoteCreatedAt + 1 without a cap. If a self-authored head was accepted near the relay's +900s boundary, a correctly clocked second device emits +901s; the relay rejects it, and retries continue deriving from the same head until wall time catches up. Apply the same bounded timestamp rule across all four sidebar sync surfaces.

CI is green at this head. I did not duplicate the CI-equivalent suite locally; these failures are source-reproduced interleavings absent from the current tests.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Re-reviewed exact current head 16aeab68b094baaba508a0f2fc91c06b0c430c1c against base ef0d2025683869418e8eee22ac5b5ac16c5198b7 after the force-rebase. Changes are still required.

All 19 files in this PR have the same Git blob IDs as at previously reviewed head 0d797b550e405eea9557e053f884533ad3dd891a, including all production sync files and tests. The only sidebar differences between the old and new repository trees are three UI files inherited from the new base; they do not touch the NIP-78 sync producers, consumers, or their direct dependencies. Consequently, the previous review's blockers survive unchanged:

1. P1: sections/stars/mutes still lack cross-window ownership for the shared durable outboxes. Manager-local generation fencing cannot stop one desktop window from overwriting another window's persisted payload or from unconditionally clearing the newer payload after its own older ACK/no-op/adopt completion. The existing tests remain single-manager and do not cover two windows sharing the key.
2. P1: sort still drops pending intent. It still has only an in-memory pendingStore, no durable outbox or failure retry, destroy() still cancels and nulls the edit, and a live remote can still cancel the timer while leaving the intent stranded.
3. P2: stars, mutes, and sort still derive created_at = max(now, lastRemoteCreatedAt + 1) without the future-drift clamp used by sections. A self-authored head accepted near the relay's +900s limit can therefore wedge subsequent writes until wall time catches up.

Please address the concrete reproductions and repair boundaries in the review on 0d797b550: #6525 (review)

Current CI is not green: Desktop Smoke E2E (2) failed at this head while several desktop jobs remain in progress. I am not using that still-unclassified failure as a separate code finding; the source blockers above independently require changes.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Requesting changes at exact head 9141fe292268e2681e65664bbeb708b732af2535.

[P1] The multi-window outbox operations are still racy

The new ownership token does not make the localStorage operations atomic. clearOutboxEntry reads and validates the stored token, then calls removeItem separately (sidebarSyncWatermark.ts:141-162). Window A can read token A, window B can write its newer {store, tokenB} envelope, and A can then remove B's value using the stale read. That still loses an unpublished edit on quit/remount.

Stars and mutes have the matching write-side race: writeChannelStarsOutbox reads the shared entry, merges in memory, then writes separately (channelStarsStorage.ts:226-237; mutes mirrors it). If two windows read before either writes, each computes a one-sided merge and the later setItem drops the other pending click. The main-store read/merge/write has the same shape (channelStarsStorage.ts:135-144).

The added multi-window tests execute whole operations sequentially, such as A write, B write, then A clear (multiWindowOutbox.test.mjs:62-167), so they cannot exercise either read/write or read/remove interleaving. localStorage provides no compare-and-delete or transactional read-modify-write primitive; the token proves what was read, not what is still stored at the destructive operation.

Please move this durability boundary to a design that does not depend on atomicity localStorage lacks, such as per-operation append-only records with owner-specific deletion, or an appropriately serialized cross-window store. Add tests that pause operations between their read and write/remove steps and verify teardown/remount preserves every unpublished intent.

The LWW comparator itself matches the relay's created_at DESC, id ASC rule; this review is blocking only on the durability race above. I reviewed read-only GitHub metadata and diff and did not check out or execute PR code.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

Requesting changes at exact head 3712e6fb2c71f03587a476c36081b1ac831a60c7 against base ef0d2025683869418e8eee22ac5b5ac16c5198b7.

Product direction: the relay is authoritative, while sidebar sections/sort use whole-blob LWW and stars/mutes use per-entry max-merge. The compatibility contract is that all four lanes converge across concurrently running builds/windows without dropping an unpublished user edit. I reviewed the four surfaces across bootstrap, live delivery, storage events, reconnect, periodic/visibility reconciliation, local edits, publish failure/ACK, destroy, remount, and community switch. I found these blockers:

[P1] Preserve the original whole-blob baseline when reconnect wakes a pending edit

useChannelSections.ts:264-276 and useChannelSortPreference.ts:257-269 fetch on reconnect and then call the public publish*() method for an already-pending edit. Those methods increment pendingGeneration and reset publishBaseline to the now-current lastRemoteHead (channelSectionsSync.ts:277-295, channelSortSync.ts:273-289).

Reproduction: window A queues an edit against H0; window B publishes H1 before A reconnects. A's reconnect fetch records H1, and applyRemote correctly defers because A is pending. The reconnect handler then re-queues A's old store, replacing its frozen H0 baseline with H1. The pre-publish fetch sees equality rather than remoteAdvancedSince, so A publishes above H1 instead of adopting the remote winner. This reverses the intended LWW decision on both whole-blob lanes.

Wake/retry the existing generation without resetting its baseline, and cover remote advancement plus reconnect while an edit is pending for both sections and sort.

[P1] Do not clear a merge-lane outbox merely because its EVENT received OK

channelStarsSync.ts:279-330 and channelMutesSync.ts:279-330 clear their own durable outbox after publishEvent resolves. But the relay returns accepted OK for a superseded NIP-33 write as a no-op, and two windows can also prefetch the same head and publish different whole blobs at the same second. Only one blob is retained.

The shared cache does not close this race: writeChannelStarsStore / writeChannelMutesStore at storage lines 140-151 overwrite the shared key without reading it, while the storage handlers at useChannelStars.ts:67-74 and useChannelMutes.ts:67-74 ignore e.newValue and reread whichever snapshot currently occupies that key. Two windows can therefore create different-channel snapshots from the same base, overwrite the cache before either storage event is processed, and independently receive successful OKs. The non-retained window then deletes the only durable record of its click. The relay, cache, and all outboxes can end with that edit absent.

After publication, clear an own outbox only after an authoritative retained-head fetch proves that head subsumes the attempted store; otherwise merge and retry. Add a two-window test where distinct edits race from the same head and both OK paths complete in either NIP-33 order.

[P1] A later old-build click can lose forever to an earlier new-build rev in the same second

channelStarsStorage.ts:59-77,188-195 and the identical mutes code normalize a missing rev from an old build to 0, then rank updatedAt before rev. If a new build publishes {updatedAt:T, rev:1, starred:true}, an old build observes it and the user unstars during the same second T, that later old-build payload has no rev and parses as rev:0. New builds deterministically retain the earlier true/rev:1; the user's later action cannot win until another click occurs in a later second.

Keeping payload version: 1 allows parsing but does not make the causal model backward-compatible. Use an explicit migration/version strategy that handles old writers, or define and test a product-accepted mixed-fleet limitation rather than claiming observed later intent cannot be lost.

[P1] An unreadable current head must block whole-blob overwrite, not fall through to publish

channelSectionsSync.ts:353-359 and channelSortSync.ts:341-347 record an existing head, but if decryption or parsing fails they return publish. A pending local blob is then signed at lastRemoteCreatedAt + 1 and replaces the only current head, even though the client could not inspect it or determine whether it should be adopted. A future schema, transient keychain/decrypt fault, or malformed payload therefore becomes destructive data loss on the next local edit.

Treat an unreadable/future head as a failed precondition: retain the durable pending edit and retry/surface the incompatibility, but do not overwrite unknown authoritative state. Add pre-publish coverage for decryption failure and unsupported payload version in sections and sort.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

REQUEST CHANGES on exact head 6078943f3be3a3fea99e86d225b79246af53f599 against ef0d2025683869418e8eee22ac5b5ac16c5198b7. I reviewed GitHub metadata, diff, and exact-head source only; I did not execute PR code.

Product contract: the relay is authoritative. Sections/sort converge as whole-blob LWW; stars/mutes converge by per-entry max-merge. A client must not overwrite relay state it could not read, because that turns a transient fetch/decrypt failure into durable cross-device data loss.

P1 — Retain edits when the authoritative pre-publish read fails

All four lanes fall through to publishing local state when the pre-publish relay query throws. Sections does so at channelSectionsSync.ts:371-416 and sort has the matching branch; stars does so at channelStarsSync.ts:210-232 and mutes mirrors it. The client therefore cannot distinguish “no head exists” from timeout/auth/socket failure.

Reproduction: device B publishes a newer sections/sort blob or independent star/mute entries after device A’s last observation; A edits while its pre-publish fetch fails. A signs above its stale watermark and can replace the unseen authoritative head, erasing B’s data. Durable outboxes and retries do not recover content that A never merged.

Return a tri-state publish decision in every lane: only a successful, genuinely absent head may publish the local store directly; fetch failure must retain the outbox and retry. Add causal tests for an unseen newer head plus rejected pre-publish fetch across sections, sort, stars, and mutes.

P1 — Stars and mutes also overwrite an existing but unreadable head

channelStarsSync.ts:220-231 records an event but returns the local store when decryption, JSON parsing, or schema validation fails; doPublish then publishes it (:315-357). Mutes has the identical path. This can destroy entries in a temporarily undecryptable or future-schema head. Bootstrap already classifies this condition as failed, while sections/sort correctly return retain for it.

Give merge-lane pre-publish reads the same retain/retry behavior and cover decrypt failure, malformed JSON, and unsupported schema. A max-merge is safe only after both operands were actually read. Dungeon law is unhelpfully strict about this.

P2 — Reject numeric revisions that cannot advance

channelStarsStorage.ts:59-69 and the matching mutes parser accept any finite non-negative integer rev, including values beyond Number.MAX_SAFE_INTEGER. Local clicks mint maxRev + 1 (useChannelStars.ts:252-267, mirrored for mutes); at sufficiently large IEEE-754 values that no longer increases. With equal timestamps/revisions, true wins (channelStarsStorage.ts:199-207), so a malformed starred:true or muted:true entry can suppress later unstar/unmute attempts indefinitely.

Require safe, bounded integers for revision and timestamp inputs, define malformed-input handling, and add a regression proving a huge revision cannot wedge a later false toggle.

The prior blockers around multi-window ownership, reconnect baselines, unreadable whole-blob heads, publish retention confirmation, and timestamp clamping are materially addressed. Exact-head CI is broadly green; it does not cover the failed-read transitions above.

Carl, an automated reviewer, commenting via Wes’s GitHub account.

REQUEST CHANGES on exact head 8bbc11b340cc584975af8f44da0036e0b9bbddc3 against base ef0d2025683869418e8eee22ac5b5ac16c5198b7. I reviewed GitHub metadata, diff, exact-head source, and the relay replacement path only; I did not execute PR code.

Product contract: the relay is authoritative within its community. Sections/sort converge as whole-blob LWW; stars/mutes converge by per-entry max-merge. Local caches and accepted publish responses must not fabricate a retained head or carry one community's preferences into another.

P1 — Confirm retention before committing sections/sort publishes

Both whole-blob managers treat any resolved publishEvent as proof their event became the retained head: sections unconditionally records the attempted tuple, clears ambiguousAttemptIds, folds it into publishBaseline, and discards the outbox at desktop/src/features/sidebar/lib/channelSectionsSync.ts:529-559; sort mirrors this at channelSortSync.ts:486-511.

That premise is false for parameterized replaceable events. At this exact head, the DB classifies a same-or-older canonical tuple as Superseded (crates/buzz-db/src/replaceable.rs:238-251), and the relay deliberately maps that to PersistResult::Duplicate rather than a rejection (crates/buzz-relay/src/handlers/command_executor.rs:161-171). The client therefore sees a successful publish promise even though its event was not retained.

Reproduction: two windows fetch head H, then both stamp H.created_at + 1; the lower event ID wins, while the higher-ID loser also receives success. The loser records its nonexistent event as the baseline and drops its durable edit. If that user edits again before reconciliation, the pre-publish fetch returns the actual same-second lower-ID winner; remoteAdvancedSince() treats it as a new competing head, and adoptRemote() discards the second edit too. One collision can erase both the raced edit and the following user action.

After ACK, fetch the authoritative retained head. Only clear/fold the attempt when that exact ID is retained; otherwise keep/retry the durable intent or adopt the true winner without poisoning the next edit's baseline. Add a two-manager same-second collision test where the losing manager makes another edit.

P1 — Scope the stars/mutes primary cache to the relay

The new outboxes are relay-scoped, but the main stars and mutes stores remain pubkey-only: channelStarsStorage.ts:32-34,91-101,147-155 and the matching channelMutesStorage.ts use buzz-channel-…:<pubkey>. Their hooks reload that same cache after every relayUrl change (useChannelStars.ts:46-57,103-107; mutes mirrors it).

Reproduction: use relay A and persist non-empty stars/mutes, then switch the same identity to relay B for the first time. B's bootstrap successfully finds no head, its B-scoped watermark is zero, and runBootstrap seed-publishes the non-empty “local” store (sidebarSyncWatermark.ts:609-615). Those entries came from A, so A's community preferences are copied into B. Sections and sort already avoid this by including normalized relayUrl in their primary cache keys.

Scope every stars/mutes primary-cache read, write, and storage-event key by normalized relay. Define a deliberate one-time policy for the legacy pubkey-only cache rather than importing it into every newly visited relay, and add an A→B absent-head regression proving no seed leak.

Exact-head CI is broadly green; the Codex security review was cancelled. These source-reproduced state transitions are not covered by that result.

Carl, an automated reviewer, commenting via Wes's GitHub account.

Changes requested

[P1] Preserve a newer edit when retained-head confirmation finds a same-second winner

Both whole-blob managers record the just-ACKed attempted tuple before awaiting authoritative confirmation (channelSectionsSync.ts:598-607, mirrored in channelSortSync.ts:555-564). A user edit queued during that await freezes the attempted tuple as its baseline (channelSectionsSync.ts:316-328, sort :312-322). If confirmation then returns a peer's lower-id same-second winner, adoptRemote records that winner but its generation guard leaves the newer edit's frozen baseline unchanged (channelSectionsSync.ts:294-301, sort :290-297). On the next cycle, remoteAdvancedSince compares winner {t,a} against attempted {t,z} and adopts the newer user edit away because a < z (channelSectionsSync.ts:414-427, sort :397-409). The outbox is cleared without ever publishing that edit.

This is reachable when a relay ACKs a superseded parameterized-replaceable write as a no-op, confirmation is delayed, and the user edits again before confirmation resolves. The collision tests queue the next edit only after confirmation/adoption has completed, so they do not exercise this interleaving.

Please fold a confirmed retained winner into the current generation's baseline when the newer edit was queued after the ACKed attempt became observable, while preserving the generation CAS for pending/UI state. Add deterministic sections and sort tests that pause confirmation, queue the second edit after ACK, then return the lower-id winner; the second edit must publish above that winner and remain durable until its own retention is confirmed.

The prior exact-head blockers around relay-scoped stars/mutes storage and non-overlapping retained-head confirmation appear addressed. This remaining race can still silently lose a sidebar sections or sort edit, so it is blocking.